In general, looks good from my POV. You just pushed it under a different PR, so it's now a completely new PR, not reopen of the older closed one.

Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Miaxos)

openraft/src/async_runtime.rs line 50 at r1 (raw file):

    type OneshotSender<T: OptionalSend>: AsyncOneshotSendExt<T> + OptionalSend + OptionalSync + Debug + Sized;

    type OneshotReceiverError: std::error::Error + OptionalSend;

Doc?

openraft/src/async_runtime.rs line 102 at r1 (raw file):

pub struct TokioRuntime;

pub struct TokioSendWrapper<T: OptionalSend>(pub tokio::sync::oneshot::Sender<T>);

Maybe simply "TokioOneShotSender"?

openraft/src/engine/command.rs line 242 at r1 (raw file):

Yes, that's a shortcoming of the derive macro - it doesn't derive if one of the generics doesn't support it. Instead, it should look at the actual members whether they support it. But, that's impossible in a proc-macro, so a special impl would be required in the compiler.

But, I'm fine with that, just maybe name them somewhat more descriptively than "f0" (maybe "channel" or "sender")?

Yeah I couldn't reopen it, not sure why.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Miaxos)

Reviewable status: 25 of 27 files reviewed, 3 unresolved discussions (waiting on @schreter)

openraft/src/async_runtime.rs line 50 at r1 (raw file):

Updated

openraft/src/async_runtime.rs line 102 at r1 (raw file):

Changed to TokioOneShotSender

openraft/src/engine/command.rs line 242 at r1 (raw file):

Changed to first_sender and second_sender

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Miaxos)

BTW there is a conflict with the main :(

Reviewed 7 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

pub struct TokioRuntime;

pub struct TokioOneShotSender<T: OptionalSend>(pub tokio::sync::oneshot::Sender<T>);

Why does it need a wrapper for the Sender?

openraft/src/core/raft_msg/mod.rs line 31 at r2 (raw file):

/// A oneshot TX to send result from `RaftCore` to external caller, e.g. `Raft::append_entries`.
pub(crate) type ResultSender<Runtime, T, E = Infallible> = <Runtime as AsyncRuntime>::OneshotSender<Result<T, E>>;

Using C: RaftTypeConfig as the type parameter could be better?

openraft/src/core/raft_msg/mod.rs line 36 at r2 (raw file):

/// TX for Vote Response
pub(crate) type VoteTx<Runtime, NID> = ResultSender<Runtime, VoteResponse<NID>>;

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

openraft/src/core/raft_msg/mod.rs line 39 at r2 (raw file):

/// TX for Append Entries Response
pub(crate) type AppendEntriesTx<Runtime, NID> = ResultSender<Runtime, AppendEntriesResponse<NID>>;

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

openraft/src/engine/command.rs line 224 at r2 (raw file):

#[derive(Debug)]
#[derive(derive_more::From)]
pub(crate) enum Respond<R, NID, N>

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

openraft/src/storage/callback.rs line 16 at r2 (raw file):

/// A oneshot callback for completion of log io operation.
pub struct LogFlushed<Runtime, NID>

If it requires more than one type parameter, use LogFlushed<C: RaftTypeConfig> would be better.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

I suppose, because of Debug requirement above?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

But then why does it have to be Debug?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

I mean, just manually implement Debug for TokioRuntime would be easier IMHO.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

True.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

OneshotSender<T> needs to be Debug, tokio::sync::oneshot::Sender<T> imp Debug only if T: Debug but it would means we need to add a constraint on T and it would "color" the whole codebase with a Debug constraint on T.

This is not due to TokioRuntime but mainly for every struct with a sender inside which implement Debug (like ValueSender) for instance.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Ah, so I was correct it was because of Debug. I didn't think about the other structs - but yes, you are right. That would make the rest quite complex. OTOH, why did it work before with Tokio sender? It's no different from before, where Tokio sender was referenced directly.

Could it be that something else is lacking Debug, like the AsyncRuntime, as @drmingdrmer pointed out? Maybe replacing the type parameter with TypeConfig as indicated elsewhere will resolve that?

Reviewed 3 of 27 files at r1, 3 of 9 files at r3, 8 of 9 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 25 of 27 files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 55 at r5 (raw file):

    let mut eng = eng();

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

Inside Openraft codebase just use AsyncRuntimeOf::<UTConfig> for short.
If needed, you can also create other type alias for the oneshot types, such as OneshotSenderOf<C>.
That will make a little bit easier:)

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 60 at r5 (raw file):

    assert!(resp.is_none());

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

AsyncRuntimeOf::<UTConfig> for short.

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 80 at r5 (raw file):

    let mut eng = eng();

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

AsyncRuntimeOf::<UTConfig> for short.

Reviewable status: 24 of 28 files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @schreter)

openraft/src/core/raft_msg/mod.rs line 31 at r2 (raw file):

Done

openraft/src/core/raft_msg/mod.rs line 36 at r2 (raw file):

Done

openraft/src/core/raft_msg/mod.rs line 39 at r2 (raw file):

Done

openraft/src/engine/command.rs line 224 at r2 (raw file):

Done

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 55 at r5 (raw file):

Done

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 60 at r5 (raw file):

Done

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 80 at r5 (raw file):

Done

openraft/src/storage/callback.rs line 16 at r2 (raw file):

Done

I needed to fix a commit message, sorry for the rebase

Reviewable status: 21 of 28 files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @schreter)

Reviewed 2 of 4 files at r6, 3 of 3 files at r7.
Reviewable status: 26 of 28 files reviewed, 4 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/engine/command.rs line 260 at r7 (raw file):

        }
    }
}

Why does it require a manual implementation of PartialEq?
C::NodeId : PartialEq and C::Node : PartialEq is always satisified:
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L11
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L62

Code quote:

impl<C> PartialEq for Respond<C>
where
    C: RaftTypeConfig,
    C::NodeId: PartialEq,
    C::Node: PartialEq,
{
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Respond::Vote(first_sender), Respond::Vote(second_sender)) => first_sender.eq(second_sender),
            (Respond::AppendEntries(first_sender), Respond::AppendEntries(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::ReceiveSnapshotChunk(first_sender), Respond::ReceiveSnapshotChunk(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::InstallSnapshot(first_sender), Respond::InstallSnapshot(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::InstallFullSnapshot(first_sender), Respond::InstallFullSnapshot(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::Initialize(first_sender), Respond::Initialize(second_sender)) => first_sender.eq(second_sender),
            _unused => false,
        }
    }
}

openraft/src/engine/command.rs line 294 at r7 (raw file):

#[derive(Debug)]
pub(crate) struct ValueSender<R, T>

What about using C: RaftTypeConfig?

It would be easier for developers to use just C instead of using C somewhere and R somewhere else.

Reviewed 1 of 27 files at r1, 1 of 4 files at r6, 5 of 5 files at r9, all commit messages.
Reviewable status: 27 of 28 files reviewed, 3 unresolved discussions (waiting on @Miaxos and @schreter)

Reviewable status: 27 of 28 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

openraft/src/engine/command.rs line 260 at r7 (raw file):

The derive macro generated by a derive(PartialEq) imply that C::AsyncRuntime: PartialEq which is not what we want, that is why I switched to the manual impl to have the same behavior as before.

// ==========================================
// Recursive expansion of the PartialEq macro
// ==========================================

impl<C: $crate::cmp::PartialEq> $crate::cmp::PartialEq for Respond<C>
where
    C: RaftTypeConfig,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::Node: $crate::cmp::PartialEq,
{
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Respond::Vote(f0_self), Respond::Vote(f0_other)) => f0_self.eq(f0_other),
            (Respond::AppendEntries(f0_self), Respond::AppendEntries(f0_other)) => f0_self.eq(f0_other),
            (Respond::ReceiveSnapshotChunk(f0_self), Respond::ReceiveSnapshotChunk(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallSnapshot(f0_self), Respond::InstallSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallFullSnapshot(f0_self), Respond::InstallFullSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::Initialize(f0_self), Respond::Initialize(f0_other)) => f0_self.eq(f0_other),
            _unused => false,
        }
    }
}

openraft/src/engine/command.rs line 294 at r7 (raw file):

Yep, will do

Reviewable status: 23 of 28 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

openraft/src/engine/command.rs line 294 at r7 (raw file):

Done

Reviewed 3 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

And what about this issue? @Miaxos

openraft/src/engine/command.rs line 260 at r7 (raw file):

I am fine with deriving PartialEq for AsyncRuntime. It's also derived for RaftTypeConfig since various structs that use it also implement PartialEq.

Manually tracking enum variant comparisons for PartialEq can introduce bugs if new variants are added without updating the corresponding PartialEq implementation.

Reviewable status: 22 of 29 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

@schreter quite tricky to answer you haha

I did some work around it to remove the wrapper, but it also introduce some constraint over some types as we can't have OneshotSender<T: !Debug>: Debug when we use a tokio::sync::oneshot::Sender<T> directly. WDYT?

openraft/src/engine/command.rs line 260 at r7 (raw file):

Yep, I understand! Done

Reviewable status: 22 of 29 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Ci is failing, because did not modify those tests yet, I still believe the wrapper was better as it did not include any additional type constraint. I'm not sure how we can do to avoid the wrapper and do not include additional type constraints.

Reviewed 1 of 9 files at r3, 5 of 9 files at r4, 2 of 3 files at r5, 1 of 4 files at r6, 3 of 3 files at r7, 2 of 5 files at r9, 3 of 3 files at r10, 1 of 2 files at r11, 1 of 2 files at r12, 6 of 6 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Not sure about this issue. In the meantime, I think that the wrapper is the lesser evil, see the other comment.

@drmingdrmer Opinions/ideas?

openraft/src/raft/mod.rs line 869 at r13 (raw file):

            + OptionalSend
            + 'static,
        V: OptionalSend + Debug + 'static,

I suppose, this is also due to the lack of channel wrapper, where the channel is only Debug if V: Debug, right?

Code quote:

        V: OptionalSend + Debug + 'static,

Reviewed 1 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

@Miaxos you are right. if OneshotSender needs Debug then wrapper is the simplest way.

openraft/src/raft/mod.rs line 869 at r13 (raw file):

This Debug won't be required if we using a wrapper for OneshotSender, right?

Reviewable status: 23 of 29 files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @schreter)

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Done

openraft/src/raft/mod.rs line 869 at r13 (raw file):

It won't be necessary with the wrapper yes.
Done

Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @schreter)

Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Miaxos)